New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
properly treat JSON data #847
Conversation
@@ -73,6 +75,10 @@ def preprocess_cell(self, cell, resources, cell_index): | |||
if mime_type in out.data: | |||
data = out.data[mime_type] | |||
|
|||
# If data was JSON, it will become a NotebookNode at this point. | |||
if isinstance(data, NotebookNode): | |||
data = json.dumps(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't actually be turning it to text, I think. That would do incorrect things for --to notebook
, for example. I think ExtractOutputPreprocessor
needs to be taught how to handle when this is a non-string (i.e. json serialize as part of writing to a file), rather than casting it to a string here.
Test ideas:
- convert to notebook and verify that json outputs are preserved as objects
- extract output with json
- make sure to include a variety of json outputs which can be dicts, lists, scalars, etc., not just dicts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
convert to notebook and verify that json outputs are preserved as objects
I believe this was already the case: we're dealing with a preprocessor that doesn't modify the cell outputs.
I have added a test to verify that it is indeed the case.
5f8295d
to
51678c2
Compare
@minrk I think this is good now. Thanks for the suggestion to check multiple data types of JSON, this was very useful. On a side-note: is a string scalar value allowed by |
OK, tests from other preprocessors are failing due to the modification of the example notebook. What's the better strategy here? Update other tests or separate the test case for this preprocessor? |
51678c2
to
720509b
Compare
@minrk I didn't see the full logs; will investigate. |
1b93428
to
a7beef2
Compare
a7beef2
to
3d1a76f
Compare
I believe it should be. Initially, JSON had to be a dict, but now I believe any JSON type should work. |
closes #846
Still TODO: tests.
I would additionally appreciate an opinion whether this fix is clean enough.